This uses `Perl::Critic` and custom PG policies for `Perl::Critic` to
analyze the code.  The custom PG policies must be under the
`Perl::Critic::Policy` to be loaded by `Perl::Critic` (they give no
alternative for that).  That means they are in the
`lib/Perl/Critic/Policy` directory. Policies corresponding to everything
that was attempted to be detected in openwebwork#1254 have been implemented except
for `randomness`. `randomness` of a problem is far more complicated than
just checking if `random`, `list_random`, etc. are called.

Basically, the code of a problem is first translated (via the
`default_preprocess_code` method of the `WeBWorK::PG::Translator`
package), then converted to a `PPI::Document` (the underlying library
that `Perl::Critic` uses), and that is passed to `Perl::Critic`.

There are some utility methods provided in the
`WeBWorK::PG::Critic::Utils` package that can be used by the PG
policies. At this point those are `getDeprecatedMacros`,
`parsePGMLBlock`, and `parseTextBlock`.

The `getDeprecatedMacros` method just lists the macros in the
`macros/deprecated` directory.

The `parsePGMLBlock` method parses PGML contents, and actually uses
PGML::Parse for the parsing, and returns `PPI::Document` representations
of the content. At this point only command blocks are returned (perl
content of `[@ ... @]` blocks), but more can be added as needed by the
policies that are created.

The `parseTextBlock` method is similar but parses
`BEGIN_TEXT`/`END_TEXT` blocks (and the ilk) using a simplified
`ev_substring` approach.  At this point only the contents of `\{ ... \}`
blocks are returned, and other elements can be added later if needed.

Unfortunately, the `parsePGMLBlock` and `parseTextBlock` methods do not
give proper positioning within the code, so the line and column numbers
of the things in the return value will not be reliable.  The only policy
that uses these at this point is the
`Perl::Critic::Policy::PG::RequireImageAltAttribute` policy and that
just reports the violations as being inside the PGML or text block the
violations are found in.

Also, the original untranslated code is passed to the policies and can
be used if needed.  The `Perl::Critic::Policy::PG::ProhibitEnddocumentMatter`
is the only policy that uses this at this point.

Note that since this is just `Perl::Critic` this also reports violations
of the core `Perl::Critic` policies (at severity level 4).  However,
there are policies that clearly don't apply to PG problem code, and so
those are disabled.  For instance, obviously `use strict` and `use
warnings` can't be called in a problem, so the
`Perl::Critic::Policy::TestingAndDebugging::RequireUseStrict` and
`Perl::Critic::Policy::TestingAndDebugging::RequireUseWarnings` policies
are disabled. The disabled policies start at line 57 of the
`WeBWorK::PG::Critic` package.  This may need tweaking as there may be
other policies that need to be disabled as well, but those are the
common violations that I have seen over the years using this for
problems that should not apply to problems (I have used a form of this
PG critic without the custom PG policies for some time now -- see
https://github.com/drgrice1/pg-language-server).

Also note that since this is just `Perl::Critic`, you can also use
`## no critic` annotations in the code to disable policy violations for
a specific line, the entire file, a specific policy on a specific line,
etc.  See https://metacpan.org/pod/Perl::Critic#BENDING-THE-RULES. For
example, if you have a problem that is in the works and are not ready to
add metadata, then add `## no critic (PG::RequireMetadata)` to the
beginning of the file, and you won't see the violations for having
missing metadata.  Note that the `bin/pg-critic.pl` script has a `-s`
or `--strict` option that ignores all `## no critic` annotations, and
forces all policies to be enforced.

The result is a reliable, versatile, and extendable approach for
critiquing problem code.

Since there was a desire to have a "problem score" and to reward good
behavior that has been implemented.  That means that not all
"violations" are bad.  Some of them are good.  The score is implemented
by setting the "explanation" of each violation as a hash which will have
the keys `score` and `explanation`.  The score will be positive if the
"violation" is good, and negative otherwise.  The `explanation` is of
course a string that would be the usual explanation.  This is a bit of a
hack since `Perl::Critic` expects the violation to be either a string or
a reference to an array of numbers (page numbers in the PBP book), but
the `explanation` method of the `Perl::Critic::Violation` object returns
the hash as is so this works to get the score from the policy.

Although, I am wondering if this "problem score" is really a good idea.
If we do start using this and make these scores public, will a low score
on a problem deter usage of the problem? It seems like this might
happen, and there are basic but quite good problems that are going to
get low scores simply because they don't need complicated macros and
code for there implementation. Will a high score really mean that a
problem is good anyway? What do we really want these scores for?  Some
sort of validation when our problems get high scores because they
utilize the things that happen to be encouraged at the time?  I am
thinking that this "problem score" idea really was NOT a good idea, and
should be removed.

If the score is removed, then there is also no point in the "positive
violations".  Those simply become a "pat on the back" for doing
something right which is really not needed (in fact that is all they
really are even with the score in my opinion).

So my proposal is to actually make this a proper critic that just shows
the things in a problem that need improvement, and remove the score and
the "positive violations". That is in my opinion what is really
important here.

This uses `Perl::Critic` and custom PG policies for `Perl::Critic` to
analyze the code.  The custom PG policies must be under the
`Perl::Critic::Policy` to be loaded by `Perl::Critic` (they give no
alternative for that).  That means they are in the
`lib/Perl/Critic/Policy` directory. Policies corresponding to everything
that was attempted to be detected in openwebwork#1254 have been implemented except
for `randomness`. `randomness` of a problem is far more complicated than
just checking if `random`, `list_random`, etc. are called.

Basically, the code of a problem is first translated (via the
`default_preprocess_code` method of the `WeBWorK::PG::Translator`
package), then converted to a `PPI::Document` (the underlying library
that `Perl::Critic` uses), and that is passed to `Perl::Critic`.

There are some utility methods provided in the
`WeBWorK::PG::Critic::Utils` package that can be used by the PG
policies. At this point those are `getDeprecatedMacros`,
`parsePGMLBlock`, and `parseTextBlock`.

The `getDeprecatedMacros` method just lists the macros in the
`macros/deprecated` directory.

The `parsePGMLBlock` method parses PGML contents, and actually uses
PGML::Parse for the parsing, and returns `PPI::Document` representations
of the content. At this point only command blocks are returned (perl
content of `[@ ... @]` blocks), but more can be added as needed by the
policies that are created.

The `parseTextBlock` method is similar but parses
`BEGIN_TEXT`/`END_TEXT` blocks (and the ilk) using a simplified
`ev_substring` approach.  At this point only the contents of `\{ ... \}`
blocks are returned, and other elements can be added later if needed.

Unfortunately, the `parsePGMLBlock` and `parseTextBlock` methods do not
give proper positioning within the code, so the line and column numbers
of the things in the return value will not be reliable.  The only policy
that uses these at this point is the
`Perl::Critic::Policy::PG::RequireImageAltAttribute` policy and that
just reports the violations as being inside the PGML or text block the
violations are found in.

Also, the original untranslated code is passed to the policies and can
be used if needed.  The `Perl::Critic::Policy::PG::ProhibitEnddocumentMatter`
is the only policy that uses this at this point.

Note that since this is just `Perl::Critic` this also reports violations
of the core `Perl::Critic` policies (at severity level 4).  However,
there are policies that clearly don't apply to PG problem code, and so
those are disabled.  For instance, obviously `use strict` and `use
warnings` can't be called in a problem, so the
`Perl::Critic::Policy::TestingAndDebugging::RequireUseStrict` and
`Perl::Critic::Policy::TestingAndDebugging::RequireUseWarnings` policies
are disabled. The disabled policies start at line 57 of the
`WeBWorK::PG::Critic` package.  This may need tweaking as there may be
other policies that need to be disabled as well, but those are the
common violations that I have seen over the years using this for
problems that should not apply to problems (I have used a form of this
PG critic without the custom PG policies for some time now -- see
https://github.com/drgrice1/pg-language-server).

Also note that since this is just `Perl::Critic`, you can also use
`## no critic` annotations in the code to disable policy violations for
a specific line, the entire file, a specific policy on a specific line,
etc.  See https://metacpan.org/pod/Perl::Critic#BENDING-THE-RULES. For
example, if you have a problem that is in the works and are not ready to
add metadata, then add `## no critic (PG::RequireMetadata)` to the
beginning of the file, and you won't see the violations for having
missing metadata.  Note that the `bin/pg-critic.pl` script has a `-s`
or `--strict` option that ignores all `## no critic` annotations, and
forces all policies to be enforced.

The result is a reliable, versatile, and extendable approach for
critiquing problem code.

Since there was a desire to have a "problem score" and to reward good
behavior that has been implemented.  That means that not all
"violations" are bad.  Some of them are good.  The score is implemented
by setting the "explanation" of each violation as a hash which will have
the keys `score` and `explanation`.  The score will be positive if the
"violation" is good, and negative otherwise.  The `explanation` is of
course a string that would be the usual explanation.  This is a bit of a
hack since `Perl::Critic` expects the violation to be either a string or
a reference to an array of numbers (page numbers in the PBP book), but
the `explanation` method of the `Perl::Critic::Violation` object returns
the hash as is so this works to get the score from the policy.

Although, I am wondering if this "problem score" is really a good idea.
If we do start using this and make these scores public, will a low score
on a problem deter usage of the problem? It seems like this might
happen, and there are basic but quite good problems that are going to
get low scores simply because they don't need complicated macros and
code for there implementation. Will a high score really mean that a
problem is good anyway? What do we really want these scores for?  Some
sort of validation when our problems get high scores because they
utilize the things that happen to be encouraged at the time?  I am
thinking that this "problem score" idea really was NOT a good idea, and
should be removed.

If the score is removed, then there is also no point in the "positive
violations".  Those simply become a "pat on the back" for doing
something right which is really not needed (in fact that is all they
really are even with the score in my opinion).

So my proposal is to actually make this a proper critic that just shows
the things in a problem that need improvement, and remove the score and
the "positive violations". That is in my opinion what is really
important here.

This uses `Perl::Critic` and custom PG policies for `Perl::Critic` to
analyze the code.  The custom PG policies must be under the
`Perl::Critic::Policy` to be loaded by `Perl::Critic` (they give no
alternative for that).  That means they are in the
`lib/Perl/Critic/Policy` directory. Policies corresponding to everything
that was attempted to be detected in openwebwork#1254 have been implemented except
for `randomness`. `randomness` of a problem is far more complicated than
just checking if `random`, `list_random`, etc. are called.

Basically, the code of a problem is first translated (via the
`default_preprocess_code` method of the `WeBWorK::PG::Translator`
package), then converted to a `PPI::Document` (the underlying library
that `Perl::Critic` uses), and that is passed to `Perl::Critic`.

There are some utility methods provided in the
`WeBWorK::PG::Critic::Utils` package that can be used by the PG
policies. At this point those are `getDeprecatedMacros`,
`parsePGMLBlock`, and `parseTextBlock`.

The `getDeprecatedMacros` method just lists the macros in the
`macros/deprecated` directory.

The `parsePGMLBlock` method parses PGML contents, and actually uses
PGML::Parse for the parsing, and returns `PPI::Document` representations
of the content. At this point only command blocks are returned (perl
content of `[@ ... @]` blocks), but more can be added as needed by the
policies that are created.

The `parseTextBlock` method is similar but parses
`BEGIN_TEXT`/`END_TEXT` blocks (and the ilk) using a simplified
`ev_substring` approach.  At this point only the contents of `\{ ... \}`
blocks are returned, and other elements can be added later if needed.

Unfortunately, the `parsePGMLBlock` and `parseTextBlock` methods do not
give proper positioning within the code, so the line and column numbers
of the things in the return value will not be reliable.  The only policy
that uses these at this point is the
`Perl::Critic::Policy::PG::RequireImageAltAttribute` policy and that
just reports the violations as being inside the PGML or text block the
violations are found in.

Also, the original untranslated code is passed to the policies and can
be used if needed.  The `Perl::Critic::Policy::PG::ProhibitEnddocumentMatter`
is the only policy that uses this at this point.

Note that since this is just `Perl::Critic` this also reports violations
of the core `Perl::Critic` policies (at severity level 4).  However,
there are policies that clearly don't apply to PG problem code, and so
those are disabled.  For instance, obviously `use strict` and `use
warnings` can't be called in a problem, so the
`Perl::Critic::Policy::TestingAndDebugging::RequireUseStrict` and
`Perl::Critic::Policy::TestingAndDebugging::RequireUseWarnings` policies
are disabled. The disabled policies start at line 57 of the
`WeBWorK::PG::Critic` package.  This may need tweaking as there may be
other policies that need to be disabled as well, but those are the
common violations that I have seen over the years using this for
problems that should not apply to problems (I have used a form of this
PG critic without the custom PG policies for some time now -- see
https://github.com/drgrice1/pg-language-server).

Also note that since this is just `Perl::Critic`, you can also use
`## no critic` annotations in the code to disable policy violations for
a specific line, the entire file, a specific policy on a specific line,
etc.  See https://metacpan.org/pod/Perl::Critic#BENDING-THE-RULES. For
example, if you have a problem that is in the works and are not ready to
add metadata, then add `## no critic (PG::RequireMetadata)` to the
beginning of the file, and you won't see the violations for having
missing metadata.  Note that the `bin/pg-critic.pl` script has a `-s`
or `--strict` option that ignores all `## no critic` annotations, and
forces all policies to be enforced.

The result is a reliable, versatile, and extendable approach for
critiquing problem code.

Since there was a desire to have a "problem score" and to reward good
behavior that has been implemented.  That means that not all
"violations" are bad.  Some of them are good.  The score is implemented
by setting the "explanation" of each violation as a hash which will have
the keys `score` and `explanation`.  The score will be positive if the
"violation" is good, and negative otherwise.  The `explanation` is of
course a string that would be the usual explanation.  This is a bit of a
hack since `Perl::Critic` expects the violation to be either a string or
a reference to an array of numbers (page numbers in the PBP book), but
the `explanation` method of the `Perl::Critic::Violation` object returns
the hash as is so this works to get the score from the policy.

Although, I am wondering if this "problem score" is really a good idea.
If we do start using this and make these scores public, will a low score
on a problem deter usage of the problem? It seems like this might
happen, and there are basic but quite good problems that are going to
get low scores simply because they don't need complicated macros and
code for there implementation. Will a high score really mean that a
problem is good anyway? What do we really want these scores for?  Some
sort of validation when our problems get high scores because they
utilize the things that happen to be encouraged at the time?  I am
thinking that this "problem score" idea really was NOT a good idea, and
should be removed.

If the score is removed, then there is also no point in the "positive
violations".  Those simply become a "pat on the back" for doing
something right which is really not needed (in fact that is all they
really are even with the score in my opinion).

So my proposal is to actually make this a proper critic that just shows
the things in a problem that need improvement, and remove the score and
the "positive violations". That is in my opinion what is really
important here.

This uses `Perl::Critic` and custom PG policies for `Perl::Critic` to
analyze the code.  The custom PG policies must be under the
`Perl::Critic::Policy` to be loaded by `Perl::Critic` (they give no
alternative for that).  That means they are in the
`lib/Perl/Critic/Policy` directory. Policies corresponding to everything
that was attempted to be detected in openwebwork#1254 have been implemented except
for `randomness`. `randomness` of a problem is far more complicated than
just checking if `random`, `list_random`, etc. are called.

Basically, the code of a problem is first translated (via the
`default_preprocess_code` method of the `WeBWorK::PG::Translator`
package), then converted to a `PPI::Document` (the underlying library
that `Perl::Critic` uses), and that is passed to `Perl::Critic`.

There are some utility methods provided in the
`WeBWorK::PG::Critic::Utils` package that can be used by the PG
policies. At this point those are `getDeprecatedMacros`,
`parsePGMLBlock`, and `parseTextBlock`.

The `getDeprecatedMacros` method just lists the macros in the
`macros/deprecated` directory.

The `parsePGMLBlock` method parses PGML contents, and actually uses
PGML::Parse for the parsing, and returns `PPI::Document` representations
of the content. At this point only command blocks are returned (perl
content of `[@ ... @]` blocks), but more can be added as needed by the
policies that are created.

The `parseTextBlock` method is similar but parses
`BEGIN_TEXT`/`END_TEXT` blocks (and the ilk) using a simplified
`ev_substring` approach.  At this point only the contents of `\{ ... \}`
blocks are returned, and other elements can be added later if needed.

Unfortunately, the `parsePGMLBlock` and `parseTextBlock` methods do not
give proper positioning within the code, so the line and column numbers
of the things in the return value will not be reliable.  The only policy
that uses these at this point is the
`Perl::Critic::Policy::PG::RequireImageAltAttribute` policy and that
just reports the violations as being inside the PGML or text block the
violations are found in.

Also, the original untranslated code is passed to the policies and can
be used if needed.  The `Perl::Critic::Policy::PG::ProhibitEnddocumentMatter`
is the only policy that uses this at this point.

Note that since this is just `Perl::Critic` this also reports violations
of the core `Perl::Critic` policies (at severity level 4).  However,
there are policies that clearly don't apply to PG problem code, and so
those are disabled.  For instance, obviously `use strict` and `use
warnings` can't be called in a problem, so the
`Perl::Critic::Policy::TestingAndDebugging::RequireUseStrict` and
`Perl::Critic::Policy::TestingAndDebugging::RequireUseWarnings` policies
are disabled. The disabled policies start at line 57 of the
`WeBWorK::PG::Critic` package.  This may need tweaking as there may be
other policies that need to be disabled as well, but those are the
common violations that I have seen over the years using this for
problems that should not apply to problems (I have used a form of this
PG critic without the custom PG policies for some time now -- see
https://github.com/drgrice1/pg-language-server).

Also note that since this is just `Perl::Critic`, you can also use
`## no critic` annotations in the code to disable policy violations for
a specific line, the entire file, a specific policy on a specific line,
etc.  See https://metacpan.org/pod/Perl::Critic#BENDING-THE-RULES. For
example, if you have a problem that is in the works and are not ready to
add metadata, then add `## no critic (PG::RequireMetadata)` to the
beginning of the file, and you won't see the violations for having
missing metadata.  Note that the `bin/pg-critic.pl` script has a `-s`
or `--strict` option that ignores all `## no critic` annotations, and
forces all policies to be enforced.

The result is a reliable, versatile, and extendable approach for
critiquing problem code.

Since there was a desire to have a "problem score" and to reward good
behavior that has been implemented.  That means that not all
"violations" are bad.  Some of them are good.  The score is implemented
by setting the "explanation" of each violation as a hash which will have
the keys `score` and `explanation`.  The score will be positive if the
"violation" is good, and negative otherwise.  The `explanation` is of
course a string that would be the usual explanation.  This is a bit of a
hack since `Perl::Critic` expects the violation to be either a string or
a reference to an array of numbers (page numbers in the PBP book), but
the `explanation` method of the `Perl::Critic::Violation` object returns
the hash as is so this works to get the score from the policy.

Although, I am wondering if this "problem score" is really a good idea.
If we do start using this and make these scores public, will a low score
on a problem deter usage of the problem? It seems like this might
happen, and there are basic but quite good problems that are going to
get low scores simply because they don't need complicated macros and
code for there implementation. Will a high score really mean that a
problem is good anyway? What do we really want these scores for?  Some
sort of validation when our problems get high scores because they
utilize the things that happen to be encouraged at the time?  I am
thinking that this "problem score" idea really was NOT a good idea, and
should be removed.

If the score is removed, then there is also no point in the "positive
violations".  Those simply become a "pat on the back" for doing
something right which is really not needed (in fact that is all they
really are even with the score in my opinion).

So my proposal is to actually make this a proper critic that just shows
the things in a problem that need improvement, and remove the score and
the "positive violations". That is in my opinion what is really
important here.

This uses `Perl::Critic` and custom PG policies for `Perl::Critic` to
analyze the code.  The custom PG policies must be under the
`Perl::Critic::Policy` to be loaded by `Perl::Critic` (they give no
alternative for that).  That means they are in the
`lib/Perl/Critic/Policy` directory. Policies corresponding to everything
that was attempted to be detected in openwebwork#1254 have been implemented except
for `randomness`. `randomness` of a problem is far more complicated than
just checking if `random`, `list_random`, etc. are called.

Basically, the code of a problem is first translated (via the
`default_preprocess_code` method of the `WeBWorK::PG::Translator`
package), then converted to a `PPI::Document` (the underlying library
that `Perl::Critic` uses), and that is passed to `Perl::Critic`.

There are some utility methods provided in the
`WeBWorK::PG::Critic::Utils` package that can be used by the PG
policies. At this point those are `getDeprecatedMacros`,
`parsePGMLBlock`, and `parseTextBlock`.

The `getDeprecatedMacros` method just lists the macros in the
`macros/deprecated` directory.

The `parsePGMLBlock` method parses PGML contents, and actually uses
PGML::Parse for the parsing, and returns `PPI::Document` representations
of the content. At this point only command blocks are returned (perl
content of `[@ ... @]` blocks), but more can be added as needed by the
policies that are created.

The `parseTextBlock` method is similar but parses
`BEGIN_TEXT`/`END_TEXT` blocks (and the ilk) using a simplified
`ev_substring` approach.  At this point only the contents of `\{ ... \}`
blocks are returned, and other elements can be added later if needed.

Unfortunately, the `parsePGMLBlock` and `parseTextBlock` methods do not
give proper positioning within the code, so the line and column numbers
of the things in the return value will not be reliable.  The only policy
that uses these at this point is the
`Perl::Critic::Policy::PG::RequireImageAltAttribute` policy and that
just reports the violations as being inside the PGML or text block the
violations are found in.

Also, the original untranslated code is passed to the policies and can
be used if needed.  The `Perl::Critic::Policy::PG::ProhibitEnddocumentMatter`
is the only policy that uses this at this point.

Note that since this is just `Perl::Critic` this also reports violations
of the core `Perl::Critic` policies (at severity level 4).  However,
there are policies that clearly don't apply to PG problem code, and so
those are disabled.  For instance, obviously `use strict` and `use
warnings` can't be called in a problem, so the
`Perl::Critic::Policy::TestingAndDebugging::RequireUseStrict` and
`Perl::Critic::Policy::TestingAndDebugging::RequireUseWarnings` policies
are disabled. The disabled policies start at line 57 of the
`WeBWorK::PG::Critic` package.  This may need tweaking as there may be
other policies that need to be disabled as well, but those are the
common violations that I have seen over the years using this for
problems that should not apply to problems (I have used a form of this
PG critic without the custom PG policies for some time now -- see
https://github.com/drgrice1/pg-language-server).

Also note that since this is just `Perl::Critic`, you can also use
`## no critic` annotations in the code to disable policy violations for
a specific line, the entire file, a specific policy on a specific line,
etc.  See https://metacpan.org/pod/Perl::Critic#BENDING-THE-RULES. For
example, if you have a problem that is in the works and are not ready to
add metadata, then add `## no critic (PG::RequireMetadata)` to the
beginning of the file, and you won't see the violations for having
missing metadata.  Note that the `bin/pg-critic.pl` script has a `-s`
or `--strict` option that ignores all `## no critic` annotations, and
forces all policies to be enforced.

The result is a reliable, versatile, and extendable approach for
critiquing problem code.

Since there was a desire to have a "problem score" and to reward good
behavior that has been implemented.  That means that not all
"violations" are bad.  Some of them are good.  The score is implemented
by setting the "explanation" of each violation as a hash which will have
the keys `score` and `explanation`.  The score will be positive if the
"violation" is good, and negative otherwise.  The `explanation` is of
course a string that would be the usual explanation.  This is a bit of a
hack since `Perl::Critic` expects the violation to be either a string or
a reference to an array of numbers (page numbers in the PBP book), but
the `explanation` method of the `Perl::Critic::Violation` object returns
the hash as is so this works to get the score from the policy.

Although, I am wondering if this "problem score" is really a good idea.
If we do start using this and make these scores public, will a low score
on a problem deter usage of the problem? It seems like this might
happen, and there are basic but quite good problems that are going to
get low scores simply because they don't need complicated macros and
code for there implementation. Will a high score really mean that a
problem is good anyway? What do we really want these scores for?  Some
sort of validation when our problems get high scores because they
utilize the things that happen to be encouraged at the time?  I am
thinking that this "problem score" idea really was NOT a good idea, and
should be removed.

If the score is removed, then there is also no point in the "positive
violations".  Those simply become a "pat on the back" for doing
something right which is really not needed (in fact that is all they
really are even with the score in my opinion).

So my proposal is to actually make this a proper critic that just shows
the things in a problem that need improvement, and remove the score and
the "positive violations". That is in my opinion what is really
important here.

This uses `Perl::Critic` and custom PG policies for `Perl::Critic` to
analyze the code.  The custom PG policies must be under the
`Perl::Critic::Policy` to be loaded by `Perl::Critic` (they give no
alternative for that).  That means they are in the
`lib/Perl/Critic/Policy` directory. Policies corresponding to everything
that was attempted to be detected in openwebwork#1254 have been implemented except
for `randomness`. `randomness` of a problem is far more complicated than
just checking if `random`, `list_random`, etc. are called.

Basically, the code of a problem is first translated (via the
`default_preprocess_code` method of the `WeBWorK::PG::Translator`
package), then converted to a `PPI::Document` (the underlying library
that `Perl::Critic` uses), and that is passed to `Perl::Critic`.

There are some utility methods provided in the
`WeBWorK::PG::Critic::Utils` package that can be used by the PG
policies. At this point those are `getDeprecatedMacros`,
`parsePGMLBlock`, and `parseTextBlock`.

The `getDeprecatedMacros` method just lists the macros in the
`macros/deprecated` directory.

The `parsePGMLBlock` method parses PGML contents, and actually uses
PGML::Parse for the parsing, and returns `PPI::Document` representations
of the content. At this point only command blocks are returned (perl
content of `[@ ... @]` blocks), but more can be added as needed by the
policies that are created.

The `parseTextBlock` method is similar but parses
`BEGIN_TEXT`/`END_TEXT` blocks (and the ilk) using a simplified
`ev_substring` approach.  At this point only the contents of `\{ ... \}`
blocks are returned, and other elements can be added later if needed.

Unfortunately, the `parsePGMLBlock` and `parseTextBlock` methods do not
give proper positioning within the code, so the line and column numbers
of the things in the return value will not be reliable.  The only policy
that uses these at this point is the
`Perl::Critic::Policy::PG::RequireImageAltAttribute` policy and that
just reports the violations as being inside the PGML or text block the
violations are found in.

Also, the original untranslated code is passed to the policies and can
be used if needed.  The `Perl::Critic::Policy::PG::ProhibitEnddocumentMatter`
is the only policy that uses this at this point.

Note that since this is just `Perl::Critic` this also reports violations
of the core `Perl::Critic` policies (at severity level 4).  However,
there are policies that clearly don't apply to PG problem code, and so
those are disabled.  For instance, obviously `use strict` and `use
warnings` can't be called in a problem, so the
`Perl::Critic::Policy::TestingAndDebugging::RequireUseStrict` and
`Perl::Critic::Policy::TestingAndDebugging::RequireUseWarnings` policies
are disabled. The disabled policies start at line 57 of the
`WeBWorK::PG::Critic` package.  This may need tweaking as there may be
other policies that need to be disabled as well, but those are the
common violations that I have seen over the years using this for
problems that should not apply to problems (I have used a form of this
PG critic without the custom PG policies for some time now -- see
https://github.com/drgrice1/pg-language-server).

Also note that since this is just `Perl::Critic`, you can also use
`## no critic` annotations in the code to disable policy violations for
a specific line, the entire file, a specific policy on a specific line,
etc.  See https://metacpan.org/pod/Perl::Critic#BENDING-THE-RULES. For
example, if you have a problem that is in the works and are not ready to
add metadata, then add `## no critic (PG::RequireMetadata)` to the
beginning of the file, and you won't see the violations for having
missing metadata.  Note that the `bin/pg-critic.pl` script has a `-s`
or `--strict` option that ignores all `## no critic` annotations, and
forces all policies to be enforced.

The result is a reliable, versatile, and extendable approach for
critiquing problem code.

Since there was a desire to have a "problem score" and to reward good
behavior that has been implemented.  That means that not all
"violations" are bad.  Some of them are good.  The score is implemented
by setting the "explanation" of each violation as a hash which will have
the keys `score` and `explanation`.  The score will be positive if the
"violation" is good, and negative otherwise.  The `explanation` is of
course a string that would be the usual explanation.  This is a bit of a
hack since `Perl::Critic` expects the violation to be either a string or
a reference to an array of numbers (page numbers in the PBP book), but
the `explanation` method of the `Perl::Critic::Violation` object returns
the hash as is so this works to get the score from the policy.

Although, I am wondering if this "problem score" is really a good idea.
If we do start using this and make these scores public, will a low score
on a problem deter usage of the problem? It seems like this might
happen, and there are basic but quite good problems that are going to
get low scores simply because they don't need complicated macros and
code for there implementation. Will a high score really mean that a
problem is good anyway? What do we really want these scores for?  Some
sort of validation when our problems get high scores because they
utilize the things that happen to be encouraged at the time?  I am
thinking that this "problem score" idea really was NOT a good idea, and
should be removed.

If the score is removed, then there is also no point in the "positive
violations".  Those simply become a "pat on the back" for doing
something right which is really not needed (in fact that is all they
really are even with the score in my opinion).

So my proposal is to actually make this a proper critic that just shows
the things in a problem that need improvement, and remove the score and
the "positive violations". That is in my opinion what is really
important here.

This uses `Perl::Critic` and custom PG policies for `Perl::Critic` to
analyze the code.  The custom PG policies must be under the
`Perl::Critic::Policy` to be loaded by `Perl::Critic` (they give no
alternative for that).  That means they are in the
`lib/Perl/Critic/Policy` directory. Policies corresponding to everything
that was attempted to be detected in openwebwork#1254 have been implemented except
for `randomness`. `randomness` of a problem is far more complicated than
just checking if `random`, `list_random`, etc. are called.

Basically, the code of a problem is first translated (via the
`default_preprocess_code` method of the `WeBWorK::PG::Translator`
package), then converted to a `PPI::Document` (the underlying library
that `Perl::Critic` uses), and that is passed to `Perl::Critic`.

There are some utility methods provided in the
`WeBWorK::PG::Critic::Utils` package that can be used by the PG
policies. At this point those are `getDeprecatedMacros`,
`parsePGMLBlock`, and `parseTextBlock`.

The `getDeprecatedMacros` method just lists the macros in the
`macros/deprecated` directory.

The `parsePGMLBlock` method parses PGML contents, and actually uses
PGML::Parse for the parsing, and returns `PPI::Document` representations
of the content. At this point only command blocks are returned (perl
content of `[@ ... @]` blocks), but more can be added as needed by the
policies that are created.

The `parseTextBlock` method is similar but parses
`BEGIN_TEXT`/`END_TEXT` blocks (and the ilk) using a simplified
`ev_substring` approach.  At this point only the contents of `\{ ... \}`
blocks are returned, and other elements can be added later if needed.

Unfortunately, the `parsePGMLBlock` and `parseTextBlock` methods do not
give proper positioning within the code, so the line and column numbers
of the things in the return value will not be reliable.  The only policy
that uses these at this point is the
`Perl::Critic::Policy::PG::RequireImageAltAttribute` policy and that
just reports the violations as being inside the PGML or text block the
violations are found in.

Also, the original untranslated code is passed to the policies and can
be used if needed.  The `Perl::Critic::Policy::PG::ProhibitEnddocumentMatter`
is the only policy that uses this at this point.

Note that since this is just `Perl::Critic` this also reports violations
of the core `Perl::Critic` policies (at severity level 4).  However,
there are policies that clearly don't apply to PG problem code, and so
those are disabled.  For instance, obviously `use strict` and `use
warnings` can't be called in a problem, so the
`Perl::Critic::Policy::TestingAndDebugging::RequireUseStrict` and
`Perl::Critic::Policy::TestingAndDebugging::RequireUseWarnings` policies
are disabled. The disabled policies start at line 57 of the
`WeBWorK::PG::Critic` package.  This may need tweaking as there may be
other policies that need to be disabled as well, but those are the
common violations that I have seen over the years using this for
problems that should not apply to problems (I have used a form of this
PG critic without the custom PG policies for some time now -- see
https://github.com/drgrice1/pg-language-server).

Also note that since this is just `Perl::Critic`, you can also use
`## no critic` annotations in the code to disable policy violations for
a specific line, the entire file, a specific policy on a specific line,
etc.  See https://metacpan.org/pod/Perl::Critic#BENDING-THE-RULES. For
example, if you have a problem that is in the works and are not ready to
add metadata, then add `## no critic (PG::RequireMetadata)` to the
beginning of the file, and you won't see the violations for having
missing metadata.  Note that the `bin/pg-critic.pl` script has a `-s`
or `--strict` option that ignores all `## no critic` annotations, and
forces all policies to be enforced.

The result is a reliable, versatile, and extendable approach for
critiquing problem code.

Since there was a desire to have a "problem score" and to reward good
behavior that has been implemented.  That means that not all
"violations" are bad.  Some of them are good.  The score is implemented
by setting the "explanation" of each violation as a hash which will have
the keys `score` and `explanation`.  The score will be positive if the
"violation" is good, and negative otherwise.  The `explanation` is of
course a string that would be the usual explanation.  This is a bit of a
hack since `Perl::Critic` expects the violation to be either a string or
a reference to an array of numbers (page numbers in the PBP book), but
the `explanation` method of the `Perl::Critic::Violation` object returns
the hash as is so this works to get the score from the policy.

Although, I am wondering if this "problem score" is really a good idea.
If we do start using this and make these scores public, will a low score
on a problem deter usage of the problem? It seems like this might
happen, and there are basic but quite good problems that are going to
get low scores simply because they don't need complicated macros and
code for there implementation. Will a high score really mean that a
problem is good anyway? What do we really want these scores for?  Some
sort of validation when our problems get high scores because they
utilize the things that happen to be encouraged at the time?  I am
thinking that this "problem score" idea really was NOT a good idea, and
should be removed.

If the score is removed, then there is also no point in the "positive
violations".  Those simply become a "pat on the back" for doing
something right which is really not needed (in fact that is all they
really are even with the score in my opinion).

So my proposal is to actually make this a proper critic that just shows
the things in a problem that need improvement, and remove the score and
the "positive violations". That is in my opinion what is really
important here.

This uses `Perl::Critic` and custom PG policies for `Perl::Critic` to
analyze the code.  The custom PG policies must be under the
`Perl::Critic::Policy` to be loaded by `Perl::Critic` (they give no
alternative for that).  That means they are in the
`lib/Perl/Critic/Policy` directory. Policies corresponding to everything
that was attempted to be detected in openwebwork#1254 have been implemented except
for `randomness`. `randomness` of a problem is far more complicated than
just checking if `random`, `list_random`, etc. are called.

Basically, the code of a problem is first translated (via the
`default_preprocess_code` method of the `WeBWorK::PG::Translator`
package), then converted to a `PPI::Document` (the underlying library
that `Perl::Critic` uses), and that is passed to `Perl::Critic`.

There are some utility methods provided in the
`WeBWorK::PG::Critic::Utils` package that can be used by the PG
policies. At this point those are `getDeprecatedMacros`,
`parsePGMLBlock`, and `parseTextBlock`.

The `getDeprecatedMacros` method just lists the macros in the
`macros/deprecated` directory.

The `parsePGMLBlock` method parses PGML contents, and actually uses
PGML::Parse for the parsing, and returns `PPI::Document` representations
of the content. At this point only command blocks are returned (perl
content of `[@ ... @]` blocks), but more can be added as needed by the
policies that are created.

The `parseTextBlock` method is similar but parses
`BEGIN_TEXT`/`END_TEXT` blocks (and the ilk) using a simplified
`ev_substring` approach.  At this point only the contents of `\{ ... \}`
blocks are returned, and other elements can be added later if needed.

Unfortunately, the `parsePGMLBlock` and `parseTextBlock` methods do not
give proper positioning within the code, so the line and column numbers
of the things in the return value will not be reliable.  The only policy
that uses these at this point is the
`Perl::Critic::Policy::PG::RequireImageAltAttribute` policy and that
just reports the violations as being inside the PGML or text block the
violations are found in.

Also, the original untranslated code is passed to the policies and can
be used if needed.  The `Perl::Critic::Policy::PG::ProhibitEnddocumentMatter`
is the only policy that uses this at this point.

Note that since this is just `Perl::Critic` this also reports violations
of the core `Perl::Critic` policies (at severity level 4).  However,
there are policies that clearly don't apply to PG problem code, and so
those are disabled.  For instance, obviously `use strict` and `use
warnings` can't be called in a problem, so the
`Perl::Critic::Policy::TestingAndDebugging::RequireUseStrict` and
`Perl::Critic::Policy::TestingAndDebugging::RequireUseWarnings` policies
are disabled. The disabled policies start at line 57 of the
`WeBWorK::PG::Critic` package.  This may need tweaking as there may be
other policies that need to be disabled as well, but those are the
common violations that I have seen over the years using this for
problems that should not apply to problems (I have used a form of this
PG critic without the custom PG policies for some time now -- see
https://github.com/drgrice1/pg-language-server).

Also note that since this is just `Perl::Critic`, you can also use
`## no critic` annotations in the code to disable policy violations for
a specific line, the entire file, a specific policy on a specific line,
etc.  See https://metacpan.org/pod/Perl::Critic#BENDING-THE-RULES. For
example, if you have a problem that is in the works and are not ready to
add metadata, then add `## no critic (PG::RequireMetadata)` to the
beginning of the file, and you won't see the violations for having
missing metadata.  Note that the `bin/pg-critic.pl` script has a `-s`
or `--strict` option that ignores all `## no critic` annotations, and
forces all policies to be enforced.

The result is a reliable, versatile, and extendable approach for
critiquing problem code.

Since there was a desire to have a "problem score" and to reward good
behavior that has been implemented.  That means that not all
"violations" are bad.  Some of them are good.  The score is implemented
by setting the "explanation" of each violation as a hash which will have
the keys `score` and `explanation`.  The score will be positive if the
"violation" is good, and negative otherwise.  The `explanation` is of
course a string that would be the usual explanation.  This is a bit of a
hack since `Perl::Critic` expects the violation to be either a string or
a reference to an array of numbers (page numbers in the PBP book), but
the `explanation` method of the `Perl::Critic::Violation` object returns
the hash as is so this works to get the score from the policy.

Although, I am wondering if this "problem score" is really a good idea.
If we do start using this and make these scores public, will a low score
on a problem deter usage of the problem? It seems like this might
happen, and there are basic but quite good problems that are going to
get low scores simply because they don't need complicated macros and
code for there implementation. Will a high score really mean that a
problem is good anyway? What do we really want these scores for?  Some
sort of validation when our problems get high scores because they
utilize the things that happen to be encouraged at the time?  I am
thinking that this "problem score" idea really was NOT a good idea, and
should be removed.

If the score is removed, then there is also no point in the "positive
violations".  Those simply become a "pat on the back" for doing
something right which is really not needed (in fact that is all they
really are even with the score in my opinion).

So my proposal is to actually make this a proper critic that just shows
the things in a problem that need improvement, and remove the score and
the "positive violations". That is in my opinion what is really
important here.

This uses `Perl::Critic` and custom PG policies for `Perl::Critic` to
analyze the code.  The custom PG policies must be under the
`Perl::Critic::Policy` to be loaded by `Perl::Critic` (they give no
alternative for that).  That means they are in the
`lib/Perl/Critic/Policy` directory. Policies corresponding to everything
that was attempted to be detected in openwebwork#1254 have been implemented except
for `randomness`. `randomness` of a problem is far more complicated than
just checking if `random`, `list_random`, etc. are called.

Basically, the code of a problem is first translated (via the
`default_preprocess_code` method of the `WeBWorK::PG::Translator`
package), then converted to a `PPI::Document` (the underlying library
that `Perl::Critic` uses), and that is passed to `Perl::Critic`.

There are some utility methods provided in the
`WeBWorK::PG::Critic::Utils` package that can be used by the PG
policies. At this point those are `getDeprecatedMacros`,
`parsePGMLBlock`, and `parseTextBlock`.

The `getDeprecatedMacros` method just lists the macros in the
`macros/deprecated` directory.

The `parsePGMLBlock` method parses PGML contents, and actually uses
PGML::Parse for the parsing, and returns `PPI::Document` representations
of the content. At this point only command blocks are returned (perl
content of `[@ ... @]` blocks), but more can be added as needed by the
policies that are created.

The `parseTextBlock` method is similar but parses
`BEGIN_TEXT`/`END_TEXT` blocks (and the ilk) using a simplified
`ev_substring` approach.  At this point only the contents of `\{ ... \}`
blocks are returned, and other elements can be added later if needed.

Unfortunately, the `parsePGMLBlock` and `parseTextBlock` methods do not
give proper positioning within the code, so the line and column numbers
of the things in the return value will not be reliable.  The only policy
that uses these at this point is the
`Perl::Critic::Policy::PG::RequireImageAltAttribute` policy and that
just reports the violations as being inside the PGML or text block the
violations are found in.

Also, the original untranslated code is passed to the policies and can
be used if needed.  The `Perl::Critic::Policy::PG::ProhibitEnddocumentMatter`
is the only policy that uses this at this point.

Note that since this is just `Perl::Critic` this also reports violations
of the core `Perl::Critic` policies (at severity level 4).  However,
there are policies that clearly don't apply to PG problem code, and so
those are disabled.  For instance, obviously `use strict` and `use
warnings` can't be called in a problem, so the
`Perl::Critic::Policy::TestingAndDebugging::RequireUseStrict` and
`Perl::Critic::Policy::TestingAndDebugging::RequireUseWarnings` policies
are disabled. The disabled policies start at line 57 of the
`WeBWorK::PG::Critic` package.  This may need tweaking as there may be
other policies that need to be disabled as well, but those are the
common violations that I have seen over the years using this for
problems that should not apply to problems (I have used a form of this
PG critic without the custom PG policies for some time now -- see
https://github.com/drgrice1/pg-language-server).

Also note that since this is just `Perl::Critic`, you can also use
`## no critic` annotations in the code to disable policy violations for
a specific line, the entire file, a specific policy on a specific line,
etc.  See https://metacpan.org/pod/Perl::Critic#BENDING-THE-RULES. For
example, if you have a problem that is in the works and are not ready to
add metadata, then add `## no critic (PG::RequireMetadata)` to the
beginning of the file, and you won't see the violations for having
missing metadata.  Note that the `bin/pg-critic.pl` script has a `-s`
or `--strict` option that ignores all `## no critic` annotations, and
forces all policies to be enforced.

The result is a reliable, versatile, and extendable approach for
critiquing problem code.

Since there was a desire to have a "problem score" and to reward good
behavior that has been implemented.  That means that not all
"violations" are bad.  Some of them are good.  The score is implemented
by setting the "explanation" of each violation as a hash which will have
the keys `score` and `explanation`.  The score will be positive if the
"violation" is good, and negative otherwise.  The `explanation` is of
course a string that would be the usual explanation.  This is a bit of a
hack since `Perl::Critic` expects the violation to be either a string or
a reference to an array of numbers (page numbers in the PBP book), but
the `explanation` method of the `Perl::Critic::Violation` object returns
the hash as is so this works to get the score from the policy.

Although, I am wondering if this "problem score" is really a good idea.
If we do start using this and make these scores public, will a low score
on a problem deter usage of the problem? It seems like this might
happen, and there are basic but quite good problems that are going to
get low scores simply because they don't need complicated macros and
code for there implementation. Will a high score really mean that a
problem is good anyway? What do we really want these scores for?  Some
sort of validation when our problems get high scores because they
utilize the things that happen to be encouraged at the time?  I am
thinking that this "problem score" idea really was NOT a good idea, and
should be removed.

If the score is removed, then there is also no point in the "positive
violations".  Those simply become a "pat on the back" for doing
something right which is really not needed (in fact that is all they
really are even with the score in my opinion).

So my proposal is to actually make this a proper critic that just shows
the things in a problem that need improvement, and remove the score and
the "positive violations". That is in my opinion what is really
important here.

This uses `Perl::Critic` and custom PG policies for `Perl::Critic` to
analyze the code.  The custom PG policies must be under the
`Perl::Critic::Policy` to be loaded by `Perl::Critic` (they give no
alternative for that).  That means they are in the
`lib/Perl/Critic/Policy` directory. Policies corresponding to everything
that was attempted to be detected in openwebwork#1254 have been implemented except
for `randomness`. `randomness` of a problem is far more complicated than
just checking if `random`, `list_random`, etc. are called.

Basically, the code of a problem is first translated (via the
`default_preprocess_code` method of the `WeBWorK::PG::Translator`
package), then converted to a `PPI::Document` (the underlying library
that `Perl::Critic` uses), and that is passed to `Perl::Critic`.

There are some utility methods provided in the
`WeBWorK::PG::Critic::Utils` package that can be used by the PG
policies. At this point those are `getDeprecatedMacros`,
`parsePGMLBlock`, and `parseTextBlock`.

The `getDeprecatedMacros` method just lists the macros in the
`macros/deprecated` directory.

The `parsePGMLBlock` method parses PGML contents, and actually uses
PGML::Parse for the parsing, and returns `PPI::Document` representations
of the content. At this point only command blocks are returned (perl
content of `[@ ... @]` blocks), but more can be added as needed by the
policies that are created.

The `parseTextBlock` method is similar but parses
`BEGIN_TEXT`/`END_TEXT` blocks (and the ilk) using a simplified
`ev_substring` approach.  At this point only the contents of `\{ ... \}`
blocks are returned, and other elements can be added later if needed.

Unfortunately, the `parsePGMLBlock` and `parseTextBlock` methods do not
give proper positioning within the code, so the line and column numbers
of the things in the return value will not be reliable.  The only policy
that uses these at this point is the
`Perl::Critic::Policy::PG::RequireImageAltAttribute` policy and that
just reports the violations as being inside the PGML or text block the
violations are found in.

Also, the original untranslated code is passed to the policies and can
be used if needed.  The `Perl::Critic::Policy::PG::ProhibitEnddocumentMatter`
is the only policy that uses this at this point.

Note that since this is just `Perl::Critic` this also reports violations
of the core `Perl::Critic` policies (at severity level 4).  However,
there are policies that clearly don't apply to PG problem code, and so
those are disabled.  For instance, obviously `use strict` and `use
warnings` can't be called in a problem, so the
`Perl::Critic::Policy::TestingAndDebugging::RequireUseStrict` and
`Perl::Critic::Policy::TestingAndDebugging::RequireUseWarnings` policies
are disabled. The disabled policies start at line 57 of the
`WeBWorK::PG::Critic` package.  This may need tweaking as there may be
other policies that need to be disabled as well, but those are the
common violations that I have seen over the years using this for
problems that should not apply to problems (I have used a form of this
PG critic without the custom PG policies for some time now -- see
https://github.com/drgrice1/pg-language-server).

Also note that since this is just `Perl::Critic`, you can also use
`## no critic` annotations in the code to disable policy violations for
a specific line, the entire file, a specific policy on a specific line,
etc.  See https://metacpan.org/pod/Perl::Critic#BENDING-THE-RULES. For
example, if you have a problem that is in the works and are not ready to
add metadata, then add `## no critic (PG::RequireMetadata)` to the
beginning of the file, and you won't see the violations for having
missing metadata.  Note that the `bin/pg-critic.pl` script has a `-s`
or `--strict` option that ignores all `## no critic` annotations, and
forces all policies to be enforced.

The result is a reliable, versatile, and extendable approach for
critiquing problem code.

Since there was a desire to have a "problem score" and to reward good
behavior that has been implemented.  That means that not all
"violations" are bad.  Some of them are good.  The score is implemented
by setting the "explanation" of each violation as a hash which will have
the keys `score` and `explanation`.  The score will be positive if the
"violation" is good, and negative otherwise.  The `explanation` is of
course a string that would be the usual explanation.  This is a bit of a
hack since `Perl::Critic` expects the violation to be either a string or
a reference to an array of numbers (page numbers in the PBP book), but
the `explanation` method of the `Perl::Critic::Violation` object returns
the hash as is so this works to get the score from the policy.

Although, I am wondering if this "problem score" is really a good idea.
If we do start using this and make these scores public, will a low score
on a problem deter usage of the problem? It seems like this might
happen, and there are basic but quite good problems that are going to
get low scores simply because they don't need complicated macros and
code for there implementation. Will a high score really mean that a
problem is good anyway? What do we really want these scores for?  Some
sort of validation when our problems get high scores because they
utilize the things that happen to be encouraged at the time?  I am
thinking that this "problem score" idea really was NOT a good idea, and
should be removed.

If the score is removed, then there is also no point in the "positive
violations".  Those simply become a "pat on the back" for doing
something right which is really not needed (in fact that is all they
really are even with the score in my opinion).

So my proposal is to actually make this a proper critic that just shows
the things in a problem that need improvement, and remove the score and
the "positive violations". That is in my opinion what is really
important here.